gh-125400: add async generator return value by alex-dixon · Pull Request #125401 · python/cpython

alex-dixon · 2024-10-13T15:50:08Z

Issue: Allow Return Statements with Values in Asynchronous Generators #125400

📚 Documentation preview 📚: https://cpython-previews--125401.org.readthedocs.build/

Issue: Allow Return Statements with Values in Asynchronous Generators #125400

ghost · 2024-10-13T15:50:10Z

All commit authors signed the Contributor License Agreement.

bedevere-app · 2024-10-13T15:50:12Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

AlexWaygood · 2024-10-13T16:20:50Z

Thanks for writing a reference implementation for this idea as was requested in the Discourse thread; it makes it much easier to evaluate the proposal! I'm marking the PR as DO-NOT-MERGE for now, however, since a change this significant would almost certainly require a PEP in order for it to be accepted (but I think you're aware of this already!).

bedevere-app · 2024-10-13T18:31:39Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

ZeroIntensity

Some initial comments. I do realize that a lot of this is copied from the StopIteration implementation, so it might be better to ignore some of my comments for consistency (or better yet, apply my comments to StopIteration as well 😄)

Overall, I think this is nice for consistency with StopIteration, but I don't think this is particularly useful without support for yield from in async generators, because you can implement this just fine in current versions--just define an exception with a value attribute, and raise it.

ZeroIntensity · 2024-10-13T21:04:11Z

Objects/genobject.c

+        return -1;
+    }
+    if (value == NULL) {
+        value = Py_NewRef(Py_None);


Py_None is immortal, I don't think we need to incref it. There's some contention about this, though.

Suggested change

value = Py_NewRef(Py_None);

value = Py_None;

ZeroIntensity · 2024-10-13T21:07:29Z

Objects/genobject.c

+    PyObject *value = NULL;
+    if (PyErr_ExceptionMatches(PyExc_StopAsyncIteration)) {
+        PyObject *exc = PyErr_GetRaisedException();
+        value = Py_NewRef(((PyStopAsyncIterationObject *)exc)->value);
+        Py_DECREF(exc);
+    } else if (PyErr_Occurred()) {
+        return -1;
+    }


There's going to be a lot of internal reusing of _PyThreadState_GET with this approach. You can use the private API:

Suggested change

PyObject *value = NULL;

if (PyErr_ExceptionMatches(PyExc_StopAsyncIteration)) {

PyObject *exc = PyErr_GetRaisedException();

value = Py_NewRef(((PyStopAsyncIterationObject *)exc)->value);

Py_DECREF(exc);

} else if (PyErr_Occurred()) {

return -1;

}

PyObject *value = NULL;

PyThreadState *tstate = _PyThreadState_GET();

PyObject *occurred = _PyErr_Occurred(tstate);

if (PyErr_GivenExceptionMatches(occurred, PyExc_StopAsyncIteration)) {

PyObject *exc = _PyErr_GetRaisedException(tstate);

value = Py_NewRef(((PyStopAsyncIterationObject *)exc)->value);

Py_DECREF(exc);

} else if (occurred) {

return -1;

}

ZeroIntensity · 2024-10-13T21:10:32Z

Objects/genobject.c

+    if (value == NULL ||
+        (!PyTuple_Check(value) && !PyExceptionInstance_Check(value)))
+    {
+        /* Delay exception instantiation if we can */
+        PyErr_SetObject(PyExc_StopAsyncIteration, value);
+        return 0;
+    }
+    /* Construct an exception instance manually with
+     * PyObject_CallOneArg and pass it to PyErr_SetObject.
+     *
+     * We do this to handle a situation when "value" is a tuple, in which
+     * case PyErr_SetObject would set the value of StopIteration to
+     * the first element of the tuple.
+     *
+     * (See PyErr_SetObject/_PyErr_CreateException code for details.)
+     */
+    e = PyObject_CallOneArg(PyExc_StopAsyncIteration, value);
+    if (e == NULL) {
+        return -1;
+    }
+    PyErr_SetObject(PyExc_StopAsyncIteration, e);
+    Py_DECREF(e);


Same story here--use the thread state.

Suggested change

if (value == NULL ||

(!PyTuple_Check(value) && !PyExceptionInstance_Check(value)))

{

/* Delay exception instantiation if we can */

PyErr_SetObject(PyExc_StopAsyncIteration, value);

return 0;

}

/* Construct an exception instance manually with

* PyObject_CallOneArg and pass it to PyErr_SetObject.

*

* We do this to handle a situation when "value" is a tuple, in which

* case PyErr_SetObject would set the value of StopIteration to

* the first element of the tuple.

*

* (See PyErr_SetObject/_PyErr_CreateException code for details.)

*/

e = PyObject_CallOneArg(PyExc_StopAsyncIteration, value);

if (e == NULL) {

return -1;

}

PyErr_SetObject(PyExc_StopAsyncIteration, e);

Py_DECREF(e);

PyThreadState *tstate = _PyThreadState_GET();

if (value == NULL ||

(!PyTuple_Check(value) && !PyExceptionInstance_Check(value)))

{

/* Delay exception instantiation if we can */

_PyErr_SetObject(tstate, PyExc_StopAsyncIteration, value);

return 0;

}

/* Construct an exception instance manually with

* PyObject_CallOneArg and pass it to PyErr_SetObject.

*

* We do this to handle a situation when "value" is a tuple, in which

* case PyErr_SetObject would set the value of StopIteration to

* the first element of the tuple.

*

* (See PyErr_SetObject/_PyErr_CreateException code for details.)

*/

e = PyObject_CallOneArg(PyExc_StopAsyncIteration, value);

if (e == NULL) {

return -1;

}

_PyErr_SetObject(tstate, PyExc_StopAsyncIteration, e);

Py_DECREF(e);

ZeroIntensity · 2024-10-13T21:11:34Z

Objects/exceptions.c

+    Py_ssize_t size = PyTuple_GET_SIZE(args);
+    PyObject *value;
+
+    if (BaseException_init((PyBaseExceptionObject *)self, args, kwds) == -1)


Typically, the convention is to use < 0 rather than == -1

Suggested change

if (BaseException_init((PyBaseExceptionObject *)self, args, kwds) == -1)

if (BaseException_init((PyBaseExceptionObject *)self, args, kwds) < 0)

ZeroIntensity · 2024-10-13T21:13:47Z

Objects/exceptions.c

+    if (size > 0)
+        value = PyTuple_GET_ITEM(args, 0);
+    else
+        value = Py_None;
+    self->value = Py_NewRef(value);


This can get simplified a little, as None is immortal like I mentioned:

Suggested change

if (size > 0)

value = PyTuple_GET_ITEM(args, 0);

else

value = Py_None;

self->value = Py_NewRef(value);

if (size > 0) {

self->value = Py_NewRef(PyTuple_GET_ITEM(args, 0));

}

else {

self->value = Py_None;

}

ZeroIntensity · 2024-10-13T21:14:25Z

Objects/exceptions.c

+
+    if (BaseException_init((PyBaseExceptionObject *)self, args, kwds) == -1)
+        return -1;
+    Py_CLEAR(self->value);


Why CLEAR it here? It's an extra operation of setting it to NULL when nothing will touch it in between calls here anyway.

Suggested change

Py_CLEAR(self->value);

Py_DECREF(self->value);

ZeroIntensity · 2024-10-13T21:16:10Z

Objects/genobject.c

+            if (result == Py_None) {
+                PyErr_SetNone(PyExc_StopAsyncIteration);
+            }
+            else {
+                _PyGen_SetStopAsyncIterationValue(result);
+            }


Consider acquiring the thread state here, and then passing that to _PyGen_SetStopAsyncIterationValue (to omit an extra call there)

ZeroIntensity · 2024-10-13T21:18:04Z

Lib/test/test_asyncgen.py

+        async def gen():
+            yield 1
+            yield 2
+            return 3


It might be a good idea to add a dummy coroutine (something like asyncio.sleep(0) should work) here, as that needs to get yielded in a different way--in my experience with dealing with the coroutine implementation, a coroutine that awaits nothing behaves differently than one that does.

Thanks so much for your thorough and prompt review. I've updated this to include awaited calls to asyncio.sleep.

bedevere-app · 2024-10-14T14:50:14Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

pkch · 2025-02-01T00:34:07Z

Some initial comments. I do realize that a lot of this is copied from the StopIteration implementation, so it might be better to ignore some of my comments for consistency (or better yet, apply my comments to StopIteration as well 😄)

Overall, I think this is nice for consistency with StopIteration, but I don't think this is particularly useful without support for yield from in async generators, because you can implement this just fine in current versions--just define an exception with a value attribute, and raise it.

I believe the strongest argument in favor of this feature even without a yield from is in this post: https://discuss.python.org/t/allow-return-statements-with-values-in-asynchronous-generators/66886/27 IMHO, (1) being able to easily switch between sync/async is of great value, and (2) the ell library referenced there is very useful. I understand this use case might look quite niche, but generally refactoring between sync and async code isn't that rare (and would be even less rare if it wasn't so hard).

Separately from the above, IIUC, @MadcowD might have been interested in writing a reference implementation for yield from? Though I may have misread the posts on that topic, so please correct me if I'm wrong.

add async generator return value

fcabdea

alex-dixon requested review from AlexWaygood, JelleZijlstra, iritkatriel, markshannon and willingc as code owners October 13, 2024 15:50

bedevere-app bot added the awaiting review label Oct 13, 2024

bedevere-app bot mentioned this pull request Oct 13, 2024

Allow Return Statements with Values in Asynchronous Generators #125400

Open

alex-dixon marked this pull request as draft October 13, 2024 16:16

bedevere-app bot removed the awaiting review label Oct 13, 2024

AlexWaygood added the DO-NOT-MERGE label Oct 13, 2024

document StopAsyncIteration.value attribute in exceptions.rst

b3c03fa

ZeroIntensity requested changes Oct 13, 2024

View reviewed changes

bedevere-app bot added the awaiting core review label Oct 13, 2024

add sleep to test

420b5fc

JelleZijlstra removed their request for review May 4, 2025 16:16

suned mentioned this pull request Sep 17, 2025

Discussion: async suned/stateless#11

Closed

	if (BaseException_init((PyBaseExceptionObject *)self, args, kwds) == -1)
	if (BaseException_init((PyBaseExceptionObject *)self, args, kwds) < 0)

Uh oh!

Conversation

alex-dixon commented Oct 13, 2024 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Oct 13, 2024 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-app bot commented Oct 13, 2024

Uh oh!

AlexWaygood commented Oct 13, 2024

Uh oh!

bedevere-app bot commented Oct 13, 2024

Uh oh!

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-app bot commented Oct 14, 2024

Uh oh!

pkch commented Feb 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alex-dixon commented Oct 13, 2024 •

edited by bedevere-app bot

Loading

ghost commented Oct 13, 2024 •

edited by ghost

Loading

pkch commented Feb 1, 2025 •

edited

Loading